Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Switched from browserify to pure-cjs builder #1002

Merged
merged 3 commits into from
Feb 4, 2014
Merged

Switched from browserify to pure-cjs builder #1002

merged 3 commits into from
Feb 4, 2014
+26 −46

Conversation

RReverser
Copy link
Contributor

Switched to recast-based own pure-cjs CommonJS builder which allows to save ~10% of minified+gzipped library size.

Current comparison table is following (used output of grunt-compare-size):

orig orig.gz diff diff.gz filename diff % diff.gz %
391132 77972 12212 -10818 JSXTransformer.js 3% -14%
538934 110908 -5706 -2861 react-with-addons.js -1% -3%
111603 31270 -15423 -3162 react-with-addons.min.js -14% -10%
504173 103543 -2489 -2188 react.js 0% -2%
104632 29316 -14674 -2908 react.min.js -14% -10%

@RReverser
Switched from browserify to pure-cjs bundler.
bff9731
@RReverser
More optimizations and fix for JSXTransformer build
f1b7db9 
* Dropped dependency on emulation of Node.js native modules.
* Added deamdify step for JSXTransformer build.
@RReverser
Copy link
Contributor Author

Added some more optimizations + fixes for JSXTransformer build, so providing updated stats:

orig orig.gz diff diff.gz filename diff % diff.gz %
391132 77972 -26812 -20441 JSXTransformer.js -7% -26%
538934 110908 -5700 -2859 react-with-addons.js -1% -3%
111603 31270 -15423 -3162 react-with-addons.min.js -14% -10%
504173 103543 -2483 -2189 react.js 0% -2%
104632 29316 -14674 -2908 react.min.js -14% -10%

@petehunt
Copy link
Contributor

wowzers.

Can you send me a link to the react.js output file so I can take a look at what it looks like?

@RReverser
Copy link
Contributor Author

Sorry, currently not at machine I have React cloned at. You can get general idea of optimization from https://github.com/RReverser/pure-cjs/blob/master/sample/a.out.js.

Do you want me to clone and build it on this machine?

@benjamn
Copy link
Contributor

benjamn commented Jan 31, 2014

This is great!

@petehunt
Copy link
Contributor

So it looks like the name require() never appears in the output bundles, right? This would fix ForbesLindesay/umd#10 for us

@RReverser
Copy link
Contributor Author

Yep, it doesn't (that's actually the reason I decided to use different name for this function in generated code).
Here are built files: http://sdrv.ms/1dez1Uq

@andreypopp andreypopp mentioned this pull request Jan 31, 2014
Closed
@RReverser
Copy link
Contributor Author

@petehunt, could you open the link? Any thoughts on this?

@benjamn
Copy link
Contributor

benjamn commented Feb 3, 2014

We probably want to change the file names so they don't imply the use of browserify. Maybe something more generic, like grunt/config/bundle.js etc.?

@RReverser
Copy link
Contributor Author

Yeah, I didn't rename files since it wasn't the most interesting important part so you could firstly have look and make decision before I make final cosmetic changes like this.

Removed redundant uglification
86373d9 
Removed uglification for separate files since it significantly slowed down build (browserify:min became 26 sec instead of 110, same for :addonsMin) while gave economy in ~70 bytes for min+gz version.
benjamn added a commit that referenced this pull request Feb 4, 2014
@benjamn
Merge pull request #1002 from RReverser/pure-cjs
806e879 
Switch from browserify to pure-cjs bundler.
@benjamn benjamn merged commit 806e879 into facebook:master Feb 4, 2014
@zpao
Copy link
Member

zpao commented Feb 4, 2014

Just talked with @benjamn a second too late, but we're going to revert and ship 0.9 with the knowns (browserify) instead of introducing this right now. I want to cut that branch this week so hopefully we'll reland this in master right after that.

@benjamn benjamn mentioned this pull request Feb 4, 2014
Merged
@RReverser
Copy link
Contributor Author

Ok, let me know when you want to merge this again, so we could resolve merge issues if any.

@plievone
Copy link
Contributor

plievone commented Feb 8, 2014

So it looks like the name require() never appears in the output bundles, right?

@petehunt Browserify --standalone since v3.24.0 does this too with derequire.

@pieterv pieterv mentioned this pull request Feb 13, 2014
Closed
@vjeux
Copy link
Contributor

vjeux commented Feb 25, 2014

Ping, 0.9 is now live :)

@RReverser RReverser mentioned this pull request Mar 8, 2014
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@RReverser @petehunt @benjamn @zpao @plievone @vjeux